Fix casing issues with React attributes #21

jamesopstad · 2019-09-19T19:54:44Z

This aims to resolve the casing differences between hast properties and react attributes. In particular it solves issues with SVG attributes such as stroke-linecap, stroke-linejoin etc. This was discussed here - https://spectrum.chat/unified/rehype/using-an-external-parser-with-rehype-react~5d05b5d8-518a-4902-a110-bc3ec85f5666. I have attempted a generalised solution based on testing for '-' so it is possible there are exceptions that don't fit this rule.

Murderlon

Thanks for the PR @jamesopstad! However, there are still some things we need to fix before we can merge. First and foremost, the CI build is currently failing, but we can fix that by doing the following things.

Could you:

Align with the code style. So var instead of const and no arrow functions.
Make the creation of reactAttribute conditional and move it into it's own function.

if (ctx.react && info.space) {
	var hasDash = info.attribute.includes('-')
	props[hasDash ? camelCase(info.attribute) : info.attribute] = value
} else {
	props[info.attribute] = value
}

Create a test to make sure it's fixed.

jamesopstad · 2019-09-20T20:43:22Z

It's going to be a few days before I can look at this properly again. I've just discovered that the react-dom repository contains a file here mapping all possible DOM attributes to their React equivalents. I think using this and simply looking up the attribute name may be a more robust approach that avoids any possible exceptions. What do you think?

wooorm · 2019-09-20T20:52:12Z

@jamesopstad Taking a couple of days is no problem, we’re here to stay, and we appreciate any help you can give. And we also like helping newcomers.

React indeed has a nice mapping, which may be better to implement compared to simple camelcasing. I’m a bit worried about including the whole file though, especially for people not using React (this library is used for others frameworks as well, such as Vue). What are your thoughts, given this?

Murderlon · 2019-09-23T19:16:03Z

React indeed has a nice mapping, which may be better to implement compared to simple camelcasing. I’m a bit worried about including the whole file though, especially for people not using React (this library is used for others frameworks as well, such as Vue).

We can:

Create a custom camelCase function, write a test that tests all the attributes from possibleStandardNames, and don't include it in the release.
Reference the attributes without custom camel casing logic and include the file.

Inluding the (un-minified) file will increase package size from 22,5kb --> 35,5kb (+57%). Which feels like a lot for a package that isn't necessarily meant for React. On the other hand, we might run into lots of exceptions in custom camel casing.

I'd say we create the test first, as it needs to be in there either way, and try our own hand at it. If there turn out to be too many exceptions we can always reconsider.

wooorm · 2019-09-23T21:02:04Z

It involves a bit of code and thought, but I think a simple camelcase utility with an overwrites object makes sense!

jamesopstad · 2019-09-24T20:29:25Z

I've just written a test that inputs all the attributes in possibleStandardNames and compares the hast and react output names. Any mismatches are added to the exceptions array. Here is the code

var names = require('./reactNames')
var find = require('property-information/find')
var html = require('property-information/html')
var svg = require('property-information/svg')

var schemas = { html, svg }

var exceptions = []

for (schema in schemas) {
  for (name in names[schema]) {
    var reactName = names[schema][name]
    var info = find(schemas[schema], name)
    if (info.property !== reactName) {
      exceptions.push({
        input: name,
        schema,
        hastName: info.property,
        reactName
      })
    }
  }
}

console.log(exceptions)

The reactNames file contains the data from possibleStandardNames but grouped into separate objects for HTML and SVG schemas.

This test returns 44 exceptions. I will put the output here for reference. As you can see, this is a bit more complex than I thought as there isn't a predictable pattern here. Some of them aren't standard attributes though so the next question is, do you want to support all of these?

[
  {
    input: 'classid',
    schema: 'html',
    hastName: 'classId',
    reactName: 'classID'
  },
  {
    input: 'contextmenu',
    schema: 'html',
    hastName: 'contextmenu',
    reactName: 'contextMenu'
  },
  {
    input: 'dangerouslysetinnerhtml',
    schema: 'html',
    hastName: 'dangerouslysetinnerhtml',
    reactName: 'dangerouslySetInnerHTML'
  },
  {
    input: 'defaultchecked',
    schema: 'html',
    hastName: 'defaultchecked',
    reactName: 'defaultChecked'
  },
  {
    input: 'defaultvalue',
    schema: 'html',
    hastName: 'defaultvalue',
    reactName: 'defaultValue'
  },
  {
    input: 'disablepictureinpicture',
    schema: 'html',
    hastName: 'disablepictureinpicture',
    reactName: 'disablePictureInPicture'
  },
  {
    input: 'innerhtml',
    schema: 'html',
    hastName: 'innerhtml',
    reactName: 'innerHTML'
  },
  {
    input: 'itemid',
    schema: 'html',
    hastName: 'itemId',
    reactName: 'itemID'
  },
  {
    input: 'keyparams',
    schema: 'html',
    hastName: 'keyparams',
    reactName: 'keyParams'
  },
  {
    input: 'keytype',
    schema: 'html',
    hastName: 'keytype',
    reactName: 'keyType'
  },
  {
    input: 'mediagroup',
    schema: 'html',
    hastName: 'mediagroup',
    reactName: 'mediaGroup'
  },
  {
    input: 'radiogroup',
    schema: 'html',
    hastName: 'radiogroup',
    reactName: 'radioGroup'
  },
  {
    input: 'allowreorder',
    schema: 'svg',
    hastName: 'allowreorder',
    reactName: 'allowReorder'
  },
  {
    input: 'autoreverse',
    schema: 'svg',
    hastName: 'autoreverse',
    reactName: 'autoReverse'
  },
  {
    input: 'datatype',
    schema: 'svg',
    hastName: 'dataType',
    reactName: 'datatype'
  },
  {
    input: 'strokedasharray',
    schema: 'svg',
    hastName: 'strokeDashArray',
    reactName: 'strokeDasharray'
  },
  {
    input: 'stroke-dasharray',
    schema: 'svg',
    hastName: 'strokeDashArray',
    reactName: 'strokeDasharray'
  },
  {
    input: 'strokedashoffset',
    schema: 'svg',
    hastName: 'strokeDashOffset',
    reactName: 'strokeDashoffset'
  },
  {
    input: 'stroke-dashoffset',
    schema: 'svg',
    hastName: 'strokeDashOffset',
    reactName: 'strokeDashoffset'
  },
  {
    input: 'strokelinecap',
    schema: 'svg',
    hastName: 'strokeLineCap',
    reactName: 'strokeLinecap'
  },
  {
    input: 'stroke-linecap',
    schema: 'svg',
    hastName: 'strokeLineCap',
    reactName: 'strokeLinecap'
  },
  {
    input: 'strokelinejoin',
    schema: 'svg',
    hastName: 'strokeLineJoin',
    reactName: 'strokeLinejoin'
  },
  {
    input: 'stroke-linejoin',
    schema: 'svg',
    hastName: 'strokeLineJoin',
    reactName: 'strokeLinejoin'
  },
  {
    input: 'strokemiterlimit',
    schema: 'svg',
    hastName: 'strokeMiterLimit',
    reactName: 'strokeMiterlimit'
  },
  {
    input: 'stroke-miterlimit',
    schema: 'svg',
    hastName: 'strokeMiterLimit',
    reactName: 'strokeMiterlimit'
  },
  {
    input: 'suppresscontenteditablewarning',
    schema: 'svg',
    hastName: 'suppresscontenteditablewarning',
    reactName: 'suppressContentEditableWarning'
  },
  {
    input: 'suppresshydrationwarning',
    schema: 'svg',
    hastName: 'suppresshydrationwarning',
    reactName: 'suppressHydrationWarning'
  },
  {
    input: 'typeof',
    schema: 'svg',
    hastName: 'typeOf',
    reactName: 'typeof'
  },
  {
    input: 'xlinkactuate',
    schema: 'svg',
    hastName: 'xLinkActuate',
    reactName: 'xlinkActuate'
  },
  {
    input: 'xlink:actuate',
    schema: 'svg',
    hastName: 'xLinkActuate',
    reactName: 'xlinkActuate'
  },
  {
    input: 'xlinkarcrole',
    schema: 'svg',
    hastName: 'xLinkArcRole',
    reactName: 'xlinkArcrole'
  },
  {
    input: 'xlink:arcrole',
    schema: 'svg',
    hastName: 'xLinkArcRole',
    reactName: 'xlinkArcrole'
  },
  {
    input: 'xlinkhref',
    schema: 'svg',
    hastName: 'xLinkHref',
    reactName: 'xlinkHref'
  },
  {
    input: 'xlink:href',
    schema: 'svg',
    hastName: 'xLinkHref',
    reactName: 'xlinkHref'
  },
  {
    input: 'xlinkrole',
    schema: 'svg',
    hastName: 'xLinkRole',
    reactName: 'xlinkRole'
  },
  {
    input: 'xlink:role',
    schema: 'svg',
    hastName: 'xLinkRole',
    reactName: 'xlinkRole'
  },
  {
    input: 'xlinkshow',
    schema: 'svg',
    hastName: 'xLinkShow',
    reactName: 'xlinkShow'
  },
  {
    input: 'xlink:show',
    schema: 'svg',
    hastName: 'xLinkShow',
    reactName: 'xlinkShow'
  },
  {
    input: 'xlinktitle',
    schema: 'svg',
    hastName: 'xLinkTitle',
    reactName: 'xlinkTitle'
  },
  {
    input: 'xlink:title',
    schema: 'svg',
    hastName: 'xLinkTitle',
    reactName: 'xlinkTitle'
  },
  {
    input: 'xlinktype',
    schema: 'svg',
    hastName: 'xLinkType',
    reactName: 'xlinkType'
  },
  {
    input: 'xlink:type',
    schema: 'svg',
    hastName: 'xLinkType',
    reactName: 'xlinkType'
  },
  {
    input: 'xmlnsxlink',
    schema: 'svg',
    hastName: 'xmlnsXLink',
    reactName: 'xmlnsXlink'
  },
  {
    input: 'xmlns:xlink',
    schema: 'svg',
    hastName: 'xmlnsXLink',
    reactName: 'xmlnsXlink'
  }
]

wooorm · 2019-09-25T07:56:21Z

Thank you so much for investigating this! ✨

React specific

dangerouslySetInnerHTML, defaultChecked, defaultValue, innerHTML, suppress*

Some of these are React-specific: if you pass dangerouslySetInnerHTML through hast, it’ll come out with the same casing, and work with React. We don’t “fix” dangerouslysetinnerhtml and properly case it though. But I think that’s fine.

Deprecated and new attributes

mediaGroup, radioGroup, contextMenu, keyParams, keyType, disablePictureInPicture (new)

These have all been removed, sometimes without a trace, or are very new (not in the spec)
We can add them to property-information though.

Different names

xmlnsXlink, ...

If the hast property is not lowercase, that means we in hast use a different name than react does. I think these are the ones this issue is really about.
It seems to be a relatively short list.

I think it may make sense to generate a file of exceptions, from the react names, in property-information. I’m think a map of hast properties to react properties: classId -> classID.
Then we could use that file (property-information/lib/react-exceptions.js?) here.
It seems from your list that it would include about 30, 40 entries, which I think we can live with.

jamesopstad · 2019-09-27T19:02:42Z

Including the following functions reduces the exception list to 18.

function reactCamelCase(attribute) {
  return attribute.replace(/[-:](.)/g, function(match, char) {
    return char.toUpperCase()
  })
}

function toReactName(info) {
  var exception = /[-:]/.test(info.attribute)
  return exception ? reactCamelCase(info.attribute) : info.property
}

This would simply mean passing info through toReactName to get the correct name. Here are the remaining exceptions:

[
  {
    input: 'classid',
    schema: 'html',
    hastName: 'classId',
    reactName: 'classID'
  },
  {
    input: 'contextmenu',
    schema: 'html',
    hastName: 'contextmenu',
    reactName: 'contextMenu'
  },
  {
    input: 'dangerouslysetinnerhtml',
    schema: 'html',
    hastName: 'dangerouslysetinnerhtml',
    reactName: 'dangerouslySetInnerHTML'
  },
  {
    input: 'defaultchecked',
    schema: 'html',
    hastName: 'defaultchecked',
    reactName: 'defaultChecked'
  },
  {
    input: 'defaultvalue',
    schema: 'html',
    hastName: 'defaultvalue',
    reactName: 'defaultValue'
  },
  {
    input: 'disablepictureinpicture',
    schema: 'html',
    hastName: 'disablepictureinpicture',
    reactName: 'disablePictureInPicture'
  },
  {
    input: 'innerhtml',
    schema: 'html',
    hastName: 'innerhtml',
    reactName: 'innerHTML'
  },
  {
    input: 'itemid',
    schema: 'html',
    hastName: 'itemId',
    reactName: 'itemID'
  },
  {
    input: 'keyparams',
    schema: 'html',
    hastName: 'keyparams',
    reactName: 'keyParams'
  },
  {
    input: 'keytype',
    schema: 'html',
    hastName: 'keytype',
    reactName: 'keyType'
  },
  {
    input: 'mediagroup',
    schema: 'html',
    hastName: 'mediagroup',
    reactName: 'mediaGroup'
  },
  {
    input: 'radiogroup',
    schema: 'html',
    hastName: 'radiogroup',
    reactName: 'radioGroup'
  },
  {
    input: 'allowreorder',
    schema: 'svg',
    hastName: 'allowreorder',
    reactName: 'allowReorder'
  },
  {
    input: 'autoreverse',
    schema: 'svg',
    hastName: 'autoreverse',
    reactName: 'autoReverse'
  },
  {
    input: 'datatype',
    schema: 'svg',
    hastName: 'dataType',
    reactName: 'datatype'
  },
  {
    input: 'suppresscontenteditablewarning',
    schema: 'svg',
    hastName: 'suppresscontenteditablewarning',
    reactName: 'suppressContentEditableWarning'
  },
  {
    input: 'suppresshydrationwarning',
    schema: 'svg',
    hastName: 'suppresshydrationwarning',
    reactName: 'suppressHydrationWarning'
  },
  {
    input: 'typeof',
    schema: 'svg',
    hastName: 'typeOf',
    reactName: 'typeof'
  }
]

Have a look and see what you think but I'm not sure that any of these are necessary. If any of them are then we can have a very small list of additional exceptions and test if the output of toReactName is included in that list.

Happy to make the changes to the code if you wish to go ahead with this approach but I'll need some help writing a test.

Related to syntax-tree/hast-to-hyperscript#21.

wooorm · 2019-10-03T19:09:02Z

@jamesopstad Thanks for all your research! I was able to make a list of exceptions and published them as property-information@5.3.0! ✨

With this released, you should be able to change your PR to a) import that JSON file, and b) do something like this:

if (ctx.react && info.space) {
  props[hastToReact[info.property] || info.property] = value
} else {
  props[info.attribute] = value
}

...that should work! Oh and could you create a test that shows that the current state is broken? Thanks!

…rty names.

wooorm · 2019-10-07T06:57:07Z

Thanks @jamesopstad, appreciate it!

Murderlon reviewed Sep 20, 2019

View reviewed changes

Murderlon added 🐛 type/bug This is a problem 👶 semver/patch This is a backwards-compatible fix 🔍 status/open labels Sep 20, 2019

wooorm added 🙆 yes/confirmed This is confirmed and ready to be worked on and removed 🔍 status/open labels Sep 25, 2019

wooorm added a commit to wooorm/property-information that referenced this pull request Oct 3, 2019

Add support for mapping to React properties

2538efb

Related to syntax-tree/hast-to-hyperscript#21.

Imported hast-to-react to resolve casing issues with some React prope…

d7a2414

…rty names.

wooorm approved these changes Oct 7, 2019

View reviewed changes

wooorm merged commit b46d21d into syntax-tree:master Oct 7, 2019

wooorm added ⛵️ status/released and removed 🙆 yes/confirmed This is confirmed and ready to be worked on labels Oct 7, 2019

wooorm added the 💪 phase/solved Post is done label Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix casing issues with React attributes #21

Fix casing issues with React attributes #21

Uh oh!

jamesopstad commented Sep 19, 2019

Uh oh!

Murderlon left a comment

Uh oh!

jamesopstad commented Sep 20, 2019

Uh oh!

wooorm commented Sep 20, 2019

Uh oh!

Murderlon commented Sep 23, 2019

Uh oh!

wooorm commented Sep 23, 2019

Uh oh!

jamesopstad commented Sep 24, 2019

Uh oh!

wooorm commented Sep 25, 2019

Uh oh!

jamesopstad commented Sep 27, 2019

Uh oh!

wooorm commented Oct 3, 2019

Uh oh!

wooorm commented Oct 7, 2019

Uh oh!

Uh oh!

Uh oh!

Fix casing issues with React attributes #21

Fix casing issues with React attributes #21

Uh oh!

Conversation

jamesopstad commented Sep 19, 2019

Uh oh!

Murderlon left a comment

Choose a reason for hiding this comment

Uh oh!

jamesopstad commented Sep 20, 2019

Uh oh!

wooorm commented Sep 20, 2019

Uh oh!

Murderlon commented Sep 23, 2019

Uh oh!

wooorm commented Sep 23, 2019

Uh oh!

jamesopstad commented Sep 24, 2019

Uh oh!

wooorm commented Sep 25, 2019

React specific

Deprecated and new attributes

Different names

Uh oh!

jamesopstad commented Sep 27, 2019

Uh oh!

wooorm commented Oct 3, 2019

Uh oh!

wooorm commented Oct 7, 2019

Uh oh!

Uh oh!